-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[main] [Possible Break] Removed Tags[] from ITelemetryItem as this was breaking later versions of TypeScript by using the intersection type instead of union type for tags property #2258 #2269
Conversation
function setTageValue(tags: Tags | Tags[], field: string, value: string): void { | ||
// Function body | ||
if (Array.isArray(tags)) { | ||
tags.forEach(tag => setValue(tag, field, value, isString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look correct... Let me try and repo locally to see the errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, looking at the usage a bit deeper and the returned error message, I think we do the following
- Remove the
Tags[]
completely so it's just defined astags?: Tags;
as during the serialization process inSerializer.ts
we process this field with the_serializeStringMap
and if that encounters an "error" it will actually report it as an error (which is what would happen if they do actually pass an incomplete array). - Add some release notes about this possible breaking change, at the very least lets call this out in the "name" of this PR, so if someone looks at the history it will be obvious
- Leave the current "default" initializations as [] as it "seems" happy about that and the serialization is working with it -- even though technically they should be objects.
As this change will only affect npm
users and the correct "typing" is really Tags anyway along with the next release being 4.1.0
its a good time to break anyone how might be trying to pass an array (which they shouldn't).
I've already checked the 1DS code with this change and it complies as well.
extensions/applicationinsights-properties-js/src/TelemetryContext.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of references the tags[] in the API.md
And can you also put a placeholder in the RELEASES.md so that we don't forget about this potential breaking change. I'll update the title of this PR
This has changed in 3.1.0, see microsoft/ApplicationInsights-JS#2269
for #2258